MDEV-40383:innodb_gis.point_basic fails on replay by bsrikanth-mariadb · Pull Request #5446 · MariaDB/server

bsrikanth-mariadb · 2026-07-23T09:21:07Z

There are 2 problems: -

The REPLACE statement that is recorded doesn't store the
value of geometry type field correctly.
The table definition that got recorded has fields with non-null constraint,
and no default value is specified.
Also, the "REPLACE INTO" statement that gets stored in the context,
doesn't have any value specified for these non-null fields.

Solution is to: -

When using REPLACE INTO statement, store all the non-numeric values in HEX,
whenever conversion from field's charset to output's charset is lossy.
Instead of storing only the column values that were projected in the
query, store all the non-virtual column values into the recorded
REPLACE INTO statement.

Implementation details: -

Introduce a new method is_charset_conversion_lossless() in filesort.cc,
to check if the output charset to which field's data is being written to,
results in a lossless conversion. If so, non-numeric values being witten
using REPLACE INTO statement are stored in string representation,
else they are converted to HEX.
From join_read_const(), and join_read_system() methods in sql_select.cc,
re-read the const row for all the non-virtual fields in the table.
After the row is re-read and recorded, restore the table->read_set,
table->status, and the const row, to the value that was before.

gemini-code-assist · 2026-07-23T09:29:07Z

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

mariadb-pavithrapandith · 2026-07-24T06:36:08Z

Test failure is fixed with the changes

vuvova · 2026-07-24T15:57:42Z

+      if (require_quote && pref_str_in_hex && tmp.length())
+      {
+        output.append(STRING_WITH_LEN("0x"));
+        output.append_hex(tmp.ptr(), tmp.length());


you know the charset of tmp and charset of output. You must always use hex if tmp cannot be converted to output charset without losses. Otherwise you don't need hex. It's not an external property that you need to pass as an argument.

May be I should narrow down the scope of change. For Geometry types I can use HEX, and for others, existing string representation should work, as it was already taken care to use charsets.

No! This is the whole point. Geometry is just a special case of a binary string that cannot necessarily be converted to the target charset. It is not a property of a type, it's a property of the charset. You must use hex in all cases where you cannot perform a lossless conversion from the source to the target charset.

updated it now.

vuvova · 2026-07-24T15:59:19Z

+  join_read_system()/join_read_const() only fetch the columns present in
+  table->read_set. That's sufficient for execution, but when we record the
+  const row for replay it yields an incomplete REPLACE INTO -- columns that
+  are NOT NULL and have no default then depend on a relaxed sql_mode (see


just make sure all columns always have defaults and you won't need any of the below!

@vuvova , do you suggest generating default values for any possible column type? How feasible is this given that there may be something fancy like GEOMETRY?

Olernov · 2026-07-24T18:24:09Z

    {
      Field *field= *pfield;
+      /* Generated columns cannot be assigned a value in REPLACE INTO */
+      if (field->vcol_info)


This function is also used by dbug_format_row() / dbug_print_row(), so this brings in a regression for those (same applies below too)

Agree. However, I believe dbug_format_row() / dbug_print_row() are only used when debug is enabled. Since it is low risk method, we clubbed our changes with this. How, about if we always set print_names=true?

At least, I'd suggest adding an argument like print_virtual_cols to format_and_store_row(), otherwise silently dropping vcols looks unexpected

if we always set print_names=true

Yes, this looks more robust than positional printing

gemini-code-assist · 2026-07-27T12:29:09Z

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

Olernov · 2026-07-28T10:47:14Z

  return length;
 }

+static bool is_target_cs_superset(CHARSET_INFO *from_cs, CHARSET_INFO *to_cs)


This name is not descriptive, consider renaming to something like:

charset_conversion_is_lossless

can_convert_losslessly

Or even invert it and rename to charset_conversion_may_lose_data, so that the code on the call site becomes more readable:

if (require_quote && tmp.length() && charset_conversion_may_lose_data(field->charset(), output.charset()))

Please, make sure @abarkov reviews it. And may be we already have a method to do it, no need to invent a new one

Sure, tried using the function can_be_safely_converted_to() defined in sql_string.h. But, it doesn't work correctly.

There are 2 problems: - 1. The REPLACE statement that is recorded doesn't store the value of geometry type field correctly. 2. The table definition that got recorded has fields with non-null constraint, and no default value is specified. Also, the "REPLACE INTO" statement that gets stored in the context, doesn't have any value specified for these non-null fields. Solution is to: - 1. When using REPLACE INTO statement, store all the non-numeric values in HEX, whenever conversion from field's charset to output's charset is lossy. 2. Instead of storing only the column values that were projected in the query, store all the non-virtual column values into the recorded REPLACE INTO statement. Implementation details: - 1. Introduce a new method is_charset_conversion_lossless() in filesort.cc, to check if the output charset to which field's data is being written to, results in a lossless conversion. If so, non-numeric values being witten using REPLACE INTO statement are stored in string representation, else they are converted to HEX. 2. From join_read_const(), and join_read_system() methods in sql_select.cc, re-read the const row for all the non-virtual fields in the table. After the row is re-read and recorded, restore the table->read_set, table->status, and the const row, to the value that was before.

bsrikanth-mariadb added the MariaDB Corporation label Jul 23, 2026

bsrikanth-mariadb force-pushed the 13.1-MDEV-40383-innodb_gis.point_basic-fails-on-replay branch 2 times, most recently from d7ceeb9 to 12df834 Compare July 24, 2026 02:51

bsrikanth-mariadb requested a review from spetrunia July 24, 2026 09:10

vuvova reviewed Jul 24, 2026

View reviewed changes

Olernov reviewed Jul 24, 2026

View reviewed changes

Comment thread sql/filesort.cc Outdated

Olernov reviewed Jul 26, 2026

View reviewed changes

Comment thread sql/sql_select.cc Outdated

Olernov reviewed Jul 26, 2026

View reviewed changes

Comment thread sql/sql_select.cc

Olernov reviewed Jul 26, 2026

View reviewed changes

Comment thread sql/sql_select.cc Outdated

bsrikanth-mariadb force-pushed the 13.1-MDEV-40383-innodb_gis.point_basic-fails-on-replay branch from 12df834 to 4ebbfec Compare July 27, 2026 11:29

bsrikanth-mariadb marked this pull request as draft July 27, 2026 11:47

bsrikanth-mariadb force-pushed the 13.1-MDEV-40383-innodb_gis.point_basic-fails-on-replay branch 3 times, most recently from 9f50d3c to cf67fb5 Compare July 27, 2026 12:28

bsrikanth-mariadb marked this pull request as ready for review July 27, 2026 12:29

bsrikanth-mariadb force-pushed the 13.1-MDEV-40383-innodb_gis.point_basic-fails-on-replay branch from cf67fb5 to d817206 Compare July 27, 2026 12:35

Olernov reviewed Jul 28, 2026

View reviewed changes

Comment thread sql/sql_select.cc Outdated

Olernov reviewed Jul 28, 2026

View reviewed changes

Comment thread sql/sql_select.cc Outdated

Olernov reviewed Jul 28, 2026

View reviewed changes

bsrikanth-mariadb requested a review from abarkov July 28, 2026 13:23

bsrikanth-mariadb force-pushed the 13.1-MDEV-40383-innodb_gis.point_basic-fails-on-replay branch from d817206 to b4d06f3 Compare July 30, 2026 04:21

bsrikanth-mariadb marked this pull request as draft July 30, 2026 07:09

bsrikanth-mariadb force-pushed the 13.1-MDEV-40383-innodb_gis.point_basic-fails-on-replay branch from b4d06f3 to 46c4a5e Compare July 30, 2026 10:24

bsrikanth-mariadb marked this pull request as ready for review July 30, 2026 10:24

Uh oh!

Uh oh!

Conversation

bsrikanth-mariadb commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist Bot commented Jul 23, 2026

Uh oh!

mariadb-pavithrapandith commented Jul 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Olernov Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Olernov Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist Bot commented Jul 27, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsrikanth-mariadb Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

bsrikanth-mariadb commented Jul 23, 2026 •

edited

Loading

Olernov Jul 24, 2026 •

edited

Loading

Olernov Jul 26, 2026 •

edited

Loading

bsrikanth-mariadb Jul 30, 2026 •

edited

Loading